Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send ratings of search parties in queue #1020

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

Conversation

BlackYps
Copy link
Collaborator

@BlackYps BlackYps commented Jun 8, 2024

Sends the ratings of search parties in a queue. This is a step to bring back the indicator that the python client had, but this time extended for all matchmaker queues.
We send just the rating, so the client can decide on its own how big of a range it wants to allow to display the indicator as a successful match also depends on the deviation of the player seeing the notification

@@ -288,8 +290,7 @@ def to_dict(self):
ndigits=2
),
"num_players": self.num_players,
"boundary_80s": [search.boundary_80 for search in self._queue.keys()],
"boundary_75s": [search.boundary_75 for search in self._queue.keys()],
"boundaries": boundaries,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just remove the old keys as that would be a breaking change in the protocol. The best thing to do would be to leave them, add a # DEPRECATED: comment indicating that they will be removed eventually, and add the new boundaries key in addition to the existing ones.

Somehow I thought they were already marked as deprecated though.

Copy link
Contributor

@Gatsik Gatsik Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Askaholic @BlackYps What do you think about sending a list (set) of means instead of boundaries? Then the client can decide for itself what range it wants to use

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a good idea

Copy link
Collaborator

@Askaholic Askaholic Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that sounds better to me too. I guess another thing to consider is should we be sending the party information in some way or just sending a flat list of means?

Or another idea would be to send something similar to a histogram to prevent the message from scaling with the number of players in queue. Something like this where the means are just rounded to the nearest 100:

{
    "summary_means": {
        400: 1,
        600: 2,
        1000: 4,
        1400: 1,
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right we should probably send the party mean instead of each players rating as the partys are matched by their average resting only. If a 1k and a 2k form a party they need other 1500s in the queue to match.

We don't need the number of players in a chunk, only if a chunk is populated at all. We could do it like this but 100 is probably a bit too coarse. How big of a deal is it anyway when the message size varies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not removing the old keys to stay protocol compatible also imply that they should still send the same info?
Breaking changes were planned for v2 of the server, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message size varying is fine, I want to make sure however that we think about the different ways that we could be encoding the information that the client needs and sending it in the most byte efficient way. More bytes means more CPU load on the server and more potential to run into bandwidth issues. If we can pass the same information in a way that avoids the possibility of scaling infinitely then I say we should go with that.

The protocol versioning is more about the structure of messages and not so much about the content. If certain messages start sending empty lists that used to be non-empty, that should be fine as the client should be able to handle that case anyway. Some feature of the client might stop working by showing empty results or just not appearing anymore, but it doesn't cause the client to break (crash) due to serialization errors or trying to perform operations on an incorrect type.

@BlackYps BlackYps changed the title Send rating boundaries for all players in a search party Send ratings of players in queue Jun 10, 2024
@BlackYps BlackYps changed the title Send ratings of players in queue Send ratings of search parties in queue Jun 10, 2024
@BlackYps
Copy link
Collaborator Author

I'm not sure why I broke a seemingly unrelated test

@BlackYps BlackYps requested a review from Askaholic June 23, 2024 11:22
server/matchmaker/matchmaker_queue.py Outdated Show resolved Hide resolved
@@ -288,9 +292,12 @@ def to_dict(self):
ndigits=2
),
"num_players": self.num_players,
"ratings": sorted(ratings),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing that we've learned from experience over the years is that sending info in the form of a dict with descriptive key names is preferable over sending a list where you need to know what the data in the list means. This is because with dicts you can easily add more keys if you need to extend the message without breaking the existing functionality.

This might be fine in this case. I think we should rename ratings to something a bit more specific though as there is also a key in the player_info message called ratings which has a different structure from this. Maybe something like rating_buckets, rating_groups or something? Also would it be useful to send the granularity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe active_rating_groups? when I hear rating_buckets my first thought is that the list contains all eligible rating buckets and all buckets not listed are invalid, whatever that means.
I don't think it's useful to send the granularity. As long as the granularity is reasonably high it doesn't really matter for the client. And if we rounded to something unreasonably coarse like nearest 200 rating then the entire list would be useless anyway.

@@ -278,6 +278,10 @@ def to_dict(self):
"""
Return a fuzzy representation of the searches currently in the queue
"""
if self.team_size == 1:
ratings = {round(search.ratings[0].mean / 25) * 25 for search in self._queue.keys()}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add a config value for the granularity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for this. We should choose a reasonable value once and then there is no real reason to change it on the fly. I will extract the number into a variable though to make it less of a magical number.

@BlackYps BlackYps force-pushed the search-boundaries branch from e592d87 to c0d1ac0 Compare July 11, 2024 20:54
@BlackYps BlackYps requested a review from Askaholic August 18, 2024 19:25
@BlackYps
Copy link
Collaborator Author

I just realized something: The 1v1 rating gets adjusted for new players. So the client needs to know how this adjustment works, so it can calculate the adjusted rating of the logged in player to determine if that player is actually close to the active rating ranges.
We can implement the formula to adjust the rating in the client, but the formula depends on server config settings that can change. If we hardcode them in the client, the calculations might be off. So what can we do about this? I think the safest way would be if the server sends the adjusted rating to the client. Then the client doesn't even need to know about the adjustment calculations.

@Askaholic
Copy link
Collaborator

I agree, we should be sending whatever number the matchmaker is actually using if possible.

@BlackYps
Copy link
Collaborator Author

I opened a new issue to discuss this. #1024
We should make the required changes in a separate pull request and then revisit this one once that is merged.

@BlackYps BlackYps marked this pull request as draft August 21, 2024 19:55
@Askaholic Askaholic force-pushed the develop branch 3 times, most recently from 75a18d1 to 0a667d2 Compare December 22, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants